Skip to content

[FEATURE] Add consumeWhitespaceWithComments and redirect consumeWhitespace (backported) - #670

Closed
ziegenberg wants to merge 2 commits into
MyIntervals:v8.xfrom
ziegenberg:add-comsume-whitespacewithcomment
Closed

[FEATURE] Add consumeWhitespaceWithComments and redirect consumeWhitespace (backported)#670
ziegenberg wants to merge 2 commits into
MyIntervals:v8.xfrom
ziegenberg:add-comsume-whitespacewithcomment

Conversation

@ziegenberg

@ziegenberg ziegenberg commented Aug 25, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Having consumeWhitespace also consumes comments, and returning them was unexpected. This also "deprecates" the consumeWhitespace method and announces a breaking change for 9.0.0.

ziegenberg added a commit to ziegenberg/PHP-CSS-Parser that referenced this pull request Aug 25, 2024
@ziegenberg
[FEATURE] Add comsumeWhitespaceWithComments and redirect comsumeWhite…
62dddaf 
…space (MyIntervals#670)

Having comsumeWhitespace also consume comments and returning them was unexpected.

Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
ziegenberg added a commit to ziegenberg/PHP-CSS-Parser that referenced this pull request Aug 25, 2024
@ziegenberg
[TASK] Deprecate ParserState::comsumeWhitespace (MyIntervals#670)
b92f7c3 
Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
@ziegenberg
ziegenberg force-pushed the add-comsume-whitespacewithcomment branch from a07190b to b92f7c3 Compare August 25, 2024 17:10
ziegenberg added a commit to ziegenberg/PHP-CSS-Parser that referenced this pull request Aug 25, 2024
@ziegenberg
[FEATURE] Add comsumeWhitespaceWithComments and redirect comsumeWhite…
119b1d2 
…space (MyIntervals#670)

Having comsumeWhitespace also consume comments and returning them was unexpected.

Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
ziegenberg added a commit to ziegenberg/PHP-CSS-Parser that referenced this pull request Aug 25, 2024
@ziegenberg
[TASK] Deprecate ParserState::comsumeWhitespace (MyIntervals#670)
79b271f 
Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
@ziegenberg
ziegenberg force-pushed the add-comsume-whitespacewithcomment branch from b92f7c3 to 79b271f Compare August 25, 2024 17:11
@ziegenberg ziegenberg changed the title [FEATURE] Add comsumeWhitespaceWithComments and redirect comsumeWhitespace [FEATURE] Add consumeWhitespaceWithComments and redirect consumeWhitespace Aug 25, 2024
@ziegenberg
[FEATURE] Add comsumeWhitespaceWithComments and redirect comsumeWhite…
0fa736a 
…space (MyIntervals#670)

Having comsumeWhitespace also consume comments and returning them was unexpected.

Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
@ziegenberg
[TASK] Deprecate ParserState::comsumeWhitespace (MyIntervals#670)
d53d9ba 
Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
@ziegenberg
ziegenberg force-pushed the add-comsume-whitespacewithcomment branch from 79b271f to d53d9ba Compare August 25, 2024 17:19
@ziegenberg ziegenberg changed the title [FEATURE] Add consumeWhitespaceWithComments and redirect consumeWhitespace [FEATURE] Add consumeWhitespaceWithComments and redirect consumeWhitespace (backported) Aug 25, 2024
oliverklee
oliverklee requested changes Aug 25, 2024
View reviewed changes

@oliverklee oliverklee left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Could you also add a test for the new method? (Copying and modifying the test for the other method should be fine.)

Comment thread src/Parsing/ParserState.php
@ziegenberg

ziegenberg commented Aug 25, 2024

Copy link
Copy Markdown
Contributor Author

Thanks! Could you also add a test for the new method? (Copying and modifying the test for the other method should be fine.)

@ziegenberg ziegenberg closed this Aug 25, 2024
@ziegenberg
ziegenberg deleted the add-comsume-whitespacewithcomment branch August 25, 2024 21:19
@ziegenberg
ziegenberg restored the add-comsume-whitespacewithcomment branch August 25, 2024 21:19
@ziegenberg ziegenberg reopened this Aug 25, 2024
@ziegenberg

ziegenberg commented Aug 25, 2024

Copy link
Copy Markdown
Contributor Author

Sorry, clicked the wrong buttons...

@oliverklee

oliverklee commented Aug 25, 2024

Copy link
Copy Markdown
Collaborator

This should go into main first. Would you be willing to create a new PR targeting main? (If I switch the target of this PR, I fear that this would create a bunch of merge conflicts.)

@oliverklee

oliverklee commented Aug 26, 2024

Copy link
Copy Markdown
Collaborator

And (as far as I see) as this isn't necessary for the bugfix (please correct me if I'm wrong) and does not provide any user-/developer facing new feature, I'd prefer this to go into main only. Would you be okay with this, @ziegenberg?

@JakeQZ

JakeQZ commented Feb 10, 2026

Copy link
Copy Markdown
Collaborator

consumeWhiteSpace() was changed in #1477 to return the whitespace consumed rather than the comments, with an optional $comments parameter to be filled in with the comments, akin to the other ParserState methods.

ParserState has been marked @internal since v8.7 and is no longer part of the public API.

If there becomes a need somewhere to consume whitespace but not comments, a method can be implemented, but I'm not currently aware of any need.

@JakeQZ JakeQZ closed this Feb 10, 2026
@JakeQZ JakeQZ added the wontimplement Suggestions for enhancements that maintainers have agreed by consensus are not worthwhile for now label Feb 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oliverklee oliverklee oliverklee requested changes

Assignees

@ziegenberg ziegenberg

Labels

enhancement wontimplement Suggestions for enhancements that maintainers have agreed by consensus are not worthwhile for now

Projects

None yet

Milestone

9.2.0: New features, bug fixes and de...

Development

Successfully merging this pull request may close these issues.

3 participants

@ziegenberg @oliverklee @JakeQZ